Fix resume succeding prematurely by zealsham · Pull Request #1573 · payjoin/rust-payjoin

zealsham · 2026-05-22T14:18:13Z

This resolves #1545, upon resuming a session where the sender posted the original proposal but went ofline, the cli succeds prematurely and outputs "payjoin transaction detected in mempool" because the cli treats any Ok(_) as sucess.

This also addresses a subtle issue where after a certain period without tx detected in the mempool the the monitor_proposal function returned
Err("Timeout waiting for payment confirmation after 5s").
That looked like a failure even though the session should stay open in Monitor. And finally the missleading "All resumed sessions completed" message.

my initial analysis of the issue in #1545 was wrong, as the intended solution witht that did not fix it. i had LLM (gemini) help with some of the root cause analysis

Pull Request Checklist

Please confirm the following before requesting review:

I have disclosed my use of
AI
in the body of this PR.
I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

coveralls · 2026-05-22T14:27:22Z

Coverage Report for CI Build 26640184155

Coverage increased (+0.05%) to 85.357%

Details

Coverage increased (+0.05%) from the base build.
Patch coverage: 7 uncovered changes across 1 file (18 of 25 lines covered, 72.0%).
No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
payjoin-cli/src/app/v2/mod.rs	25	18	72.0%

Coverage Regressions

No coverage regressions found.

Coverage Stats


Relevant Lines:	13856
Covered Lines:	11827
Line Coverage:	85.36%
Coverage Strength:	391.26 hits per line

💛 - Coveralls

xstoicunicornx

I like how you are breaking out session level information but I think this solution is a bit over engineered and it seems unnecessary to introduce additional state.

My comments below are a bit general but feel free to take more concrete direction from my branch https://github.com/xstoicunicornx/rust-payjoin/tree/pr1573-alt

xstoicunicornx

A bit more feedback.

xstoicunicornx · 2026-05-25T23:34:48Z

            } => {
-                println!("All resumed sessions completed.");
+                let closed = results.iter().filter(|r| matches!(r, Ok(Ok(())))).count();
+                let total = spawned_recv_ids.len() + spawned_send_ids.len();


Suggested change

let total = spawned_recv_ids.len() + spawned_send_ids.len();

let total = results.len();

Why not just this? Then the spawned_recv_ids and spawned_send_ids aren't needed anymore.

xstoicunicornx · 2026-05-25T23:35:31Z

-                wait_for_stdout_match(&mut stdout, |line| {
-                    line.contains("All resumed sessions completed.")
-                }),
+                wait_for_stdout_match(&mut stdout, |line| line.contains("Resume done:")),


Suggested change

wait_for_stdout_match(&mut stdout, |line| line.contains("Resume done:")),

wait_for_stdout_match(&mut stdout, |line| line.contains("1/1 session(s) closed.")),

This is supposed to assert that the resumed session was completed, checking for "Resume done:" only asserts that summary was printed not that the session was actually closed.

xstoicunicornx · 2026-05-25T23:39:14Z

+            Err(_) => {
+                let id = persister.session_id();
+                println!(
+                    "No payjoin transaction seen yet (waited {timeout_duration:?}). Session {id} \


Suggested change

"No payjoin transaction seen yet (waited {timeout_duration:?}). Session {id} \

"No payjoin transaction seen yet, stopping (waited {timeout_duration:?} sec). Session {id} \

xstoicunicornx · 2026-05-25T23:42:39Z

+                let id = persister.session_id();
+                println!(
+                    "No payjoin transaction seen yet (waited {timeout_duration:?}). Session {id} \
+                     remains open; run `payjoin-cli resume` to keep watching."


Suggested change

remains open; run `payjoin-cli resume` to keep watching."

remains open. Run `payjoin-cli resume` to keep watching."

nit: semicolon isn't used anywhere else

thanks for catching that

spacebear21 · 2026-05-26T15:12:09Z

+                    }
+                }
+                println!(
+                    "Resume done: {closed}/{total} session(s) closed. \


I don't think printing the completed/total sessions count is useful or worth the complexity. It seems sufficient to print per-session results (session XYZ either completed, is still polling, or had an error).

spacebear21 · 2026-05-26T15:13:58Z

+            Err(_) => {
+                let id = persister.session_id();
+                println!(
+                    "No payjoin transaction seen yet (waited {timeout_duration:?}). Session {id} \
+                     remains open; run `payjoin-cli resume` to keep watching."
+                );
+                Err(anyhow!(
+                    "No payjoin transaction detected in mempool within {timeout_duration:?}"
+                ))
+            }


this change seems unrelated to the resume fix, if really necessary it should be a separate commit.

Yes , it's not related to resume fix , more like a helpful ux . i'll extract to diff commit or drop it entirely

This addresses payjoin#1545, upon resuming a session where the sender posted the original proposal but went ofline, the cli succeds prematurely and outputs "payjoin transaction detected in mempool" because the cli treats any Ok(_) as scucess. This also addresses a subtle issue where after a certain period without tx detected in the mempool the the monitor_proposal function returned Err("Timeout waiting for payment confirmation after 5s"). That looked like a failure even though the session should stay open in Monitor. And finally the missleading "All resumed sessions completed" message. Co-authored-by: xstoicunicornx <xstoicunicornx@users.noreply.github.com>

xstoicunicornx

Comments below.

xstoicunicornx · 2026-05-27T15:07:30Z

        }

-        let mut tasks = Vec::new();
+        //let mut tasks = Vec::new();


Suggested change

//let mut tasks = Vec::new();

Remove comment

that was a mistake , thanks for catching it

xstoicunicornx · 2026-05-27T15:09:14Z

+            if all_completed {
                println!("All resumed sessions completed.");
            }


Since completion is being printed session by session already I think it would be better to just print a "Done" message here to indicate the resume has concluded.

Sorry what I meant was that it should print "Done" regardless of whether all sessions were completed or not just to indicate that the resume command has finished. But tbh maybe we don't really need to print anything here anymore.

oh!, i misunderstood you. Imho telling users that all resumed session completed is helpful . The "All resumed sessions completed" message can stay as it is and then we also have a diff message that indicates the Resume command is done. I also agree that not printing anything does not affect UX adversely.

would love to hear your opinion @xstoicunicornx before i push the new changes

Given that every resumed session now prints whether the session completed or not, I feel like the user can already easily tell if all resumed session completed or not. So imo it doesn't convey any additional helpful information.

xstoicunicornx · 2026-05-27T15:16:26Z

+                let id = persister.session_id();
+                println!(
+                    "No payjoin transaction seen yet, stopping (waited {timeout_duration:?} seconds). Session {id} \
+                     remains open. run `payjoin-cli resume` to keep watching."
+                );


We don't need this anymore now that errors are printed to the user upon task completion.

xstoicunicornx · 2026-05-27T15:17:41Z

+                     remains open. run `payjoin-cli resume` to keep watching."
+                );
+                Err(anyhow!(
+                    "No payjoin transaction detected in mempool within {timeout_duration:?}"


Suggested change

"No payjoin transaction detected in mempool within {timeout_duration:?}"

"No payjoin transaction detected in mempool within {timeout_duration:?} sec, stopping."

I think this small change would make it much more clear what is happening.

xstoicunicornx · 2026-05-27T15:19:02Z

+
+    pub fn session_id(&self) -> SessionId { self.session_id.clone() }


This shouldn't be needed anymore.

xstoicunicornx · 2026-05-27T15:22:52Z


            terminate(cli_resumer).await.expect("Failed to kill payjoin-cli");
-            assert!(res.is_some(), "Expected all resumed sessions completed");
+            assert!(res.is_some(), "Expected resume summary with all sessions closed");


Looks like this line change is leftover from previous version of this PR.

xstoicunicornx

Getting there :)

xstoicunicornx · 2026-05-28T17:44:45Z

-                    tasks.push(tokio::spawn(async move {
-                        self_clone.process_sender_session(sender_state, &sender_persister).await
-                    }));
+


nit: extra line added here

xstoicunicornx · 2026-05-28T17:47:49Z

+            if all_completed {
                println!("All resumed sessions completed.");
            }


Sorry what I meant was that it should print "Done" regardless of whether all sessions were completed or not just to indicate that the resume command has finished. But tbh maybe we don't really need to print anything here anymore.

xstoicunicornx · 2026-05-28T17:57:22Z

+            let timeout = tokio::time::Duration::from_secs(10);
+            let res = tokio::time::timeout(
+                timeout,
+                wait_for_stdout_match(&mut stdout, |line| line.contains("Done.")),


Suggested change

wait_for_stdout_match(&mut stdout, |line| line.contains("Done.")),

wait_for_stdout_match(&mut stdout, |line| {

line.starts_with("Session ") && line.ends_with(" completed."))

},

Better to key off of the print message that actually indicates the session was completed.

xstoicunicornx · 2026-05-28T18:00:31Z

                wait_for_stdout_match(&mut stdout, |line| {
                    line.contains("All resumed sessions completed.")
+                        || line.contains("No sessions to resume.")


Suggested change

wait_for_stdout_match(&mut stdout, |line| {

line.contains("All resumed sessions completed.")

|| line.contains("No sessions to resume.")

wait_for_stdout_match(&mut stdout, |line| {

line.starts_with("Session ") && line.ends_with(" completed.")

Same with this one. Plus not finding a string containing either of these no longer means that a session was not completed.

xstoicunicornx

This looks good. Just one bad suggestion that I made that needs to be reverted (sorry... I didn't realize until I ran the test locally)

xstoicunicornx · 2026-05-29T14:58:15Z

            Err(_) => Err(anyhow!(
-                "Timeout waiting for payment confirmation after {:?}",
-                timeout_duration
+                "No payjoin transaction detected in mempool within {timeout_duration:?} seconds, stopping."


Suggested change

"No payjoin transaction detected in mempool within {timeout_duration:?} seconds, stopping."

"No payjoin transaction detected in mempool within {timeout_duration:?}, stopping."

I was wrong about adding the "seconds", its already included as part of the timeout_duration.

Also terrible that i missed it myself

xstoicunicornx

utACK f3bc1ab

Solution is fairly minimal yet implements error handling and improves clarity of messages printed to the user. It also includes testing to guard against this bug in the future.

xstoicunicornx · 2026-05-30T20:00:28Z

@zealsham just updated your description to use the "resolves #issue-number" pattern which will automatically link this PR to that issue (and close that issue when this is merged).

zealsham force-pushed the premature-resume branch from 2303b41 to 56c9bb2 Compare May 22, 2026 14:19

zealsham mentioned this pull request May 22, 2026

Fix payjoin-cli receiver premature resolution #1575

Closed

2 tasks

xstoicunicornx requested changes May 23, 2026

View reviewed changes

Comment thread payjoin-cli/src/app/v2/mod.rs Outdated

Comment thread payjoin-cli/src/app/v2/mod.rs Outdated

Comment thread payjoin-cli/src/app/v2/mod.rs Outdated

zealsham force-pushed the premature-resume branch 2 times, most recently from 0809f0c to 0e3690a Compare May 25, 2026 21:20

xstoicunicornx requested changes May 25, 2026

View reviewed changes

spacebear21 requested changes May 26, 2026

View reviewed changes

zealsham force-pushed the premature-resume branch 2 times, most recently from 0ed9646 to 7779c42 Compare May 27, 2026 13:35

xstoicunicornx requested changes May 27, 2026

View reviewed changes

zealsham force-pushed the premature-resume branch from 7779c42 to 60a03aa Compare May 28, 2026 15:37

xstoicunicornx requested changes May 28, 2026

View reviewed changes

zealsham force-pushed the premature-resume branch from 60a03aa to fb01450 Compare May 29, 2026 13:31

xstoicunicornx requested changes May 29, 2026

View reviewed changes

Add descriptive message to monitor failing to detect TX in mempool

f3bc1ab

zealsham force-pushed the premature-resume branch from fb01450 to f3bc1ab Compare May 30, 2026 19:23

xstoicunicornx approved these changes May 30, 2026

View reviewed changes

	let total = spawned_recv_ids.len() + spawned_send_ids.len();
	let total = results.len();

	wait_for_stdout_match(&mut stdout, \|line\| line.contains("Resume done:")),
	wait_for_stdout_match(&mut stdout, \|line\| line.contains("1/1 session(s) closed.")),

	"No payjoin transaction seen yet (waited {timeout_duration:?}). Session {id} \
	"No payjoin transaction seen yet, stopping (waited {timeout_duration:?} sec). Session {id} \

	remains open; run `payjoin-cli resume` to keep watching."
	remains open. Run `payjoin-cli resume` to keep watching."

	"No payjoin transaction detected in mempool within {timeout_duration:?}"
	"No payjoin transaction detected in mempool within {timeout_duration:?} sec, stopping."


		pub fn session_id(&self) -> SessionId { self.session_id.clone() }

Conversation

zealsham commented May 22, 2026 • edited by xstoicunicornx Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 26640184155

Coverage increased (+0.05%) to 85.357%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

xstoicunicornx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xstoicunicornx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xstoicunicornx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xstoicunicornx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xstoicunicornx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

zealsham commented May 22, 2026 •

edited by xstoicunicornx

Loading

coveralls commented May 22, 2026 •

edited

Loading

xstoicunicornx commented May 30, 2026 •

edited

Loading